Conversation
|
Thanks for your contribution, and sorry for the delay of my reply. I don't know if you have a case where you would really need a dedicated command for them? For your info, there was 2 reasons for why there was not a dedicated command so far:
|
There was a problem hiding this comment.
I didn't put individual review comments as I don't think that the PR would be necessary as we have the "info" command with that,
but in the case that you would have a good argument for the dedicated commands, here are the issues I see with your PR:
- "list_policy" is not needed because "get_policy" already does that.
- Only bucket have policies, not the individual objects
- Your 2 new commands only output the input uri and nothing else...
(In fact, I discovered today that if you return a string to sys.exit, it will print it, so I see how you saw your acl, but we are not supposed to do that in the code, just return an error code at the end of the function)
|
I've updated the PR. To me, the explicit commands were useful. I cannot say how it is for others, but i think it's more confusing to expect the ACLs in an "info" command rather than having explicit one (i didn't find the info command 6 months ago, when I used this). But obviously this is your call :) |
fviard
left a comment
fviard
left a comment
There was a problem hiding this comment.
I'm not 100% convinced that the dedicated commands are needed, but I'm not against adding them.
But there are a few more changes to be done that you will find with this review.
Thanks a lot for the hints! To make both our efforts not worthless, i've updated everything based on your feedback but kept it to a minimum (didn't add a parameter for the raw acl output). Thanks a lot for your suggestions! |
fviard
left a comment
fviard
left a comment
There was a problem hiding this comment.
Sorry for my long delay to reply sometimes and thank you for your own reactivity.
It is almost good, but it just remains a few small things and then we will be good to go :-)
btw I tested that it works ok for the nick with aws. In fact, for the moment, we will have the nick of the owner just for the bucket and not for buckets, so your code is working well in both cases.
|
Hey, No worries - happy that we move forward :) |
|
All good now and merged, thank you! |
3 participants
I added some very simple commands to list the Policy and ACL. It seems like it's almost a design decission that this wasn't there... but since I missed it, i thought maybe others find it useful, too.
Example Usage:
python3 s3cmd -c ceph.cfg listacl s3://test3
python3 s3cmd -c ceph.cfg listacl s3://test3/test.txt
python3 s3cmd -c ceph.cfg listpolicy s3://test3
I was only able to test this on ceph unfortunately.